Skip to content

Add Unicode Bidi properties to the exported data.#6

Closed
cscott wants to merge 5 commits into
node-unicode:masterfrom
cscott:bidi
Closed

Add Unicode Bidi properties to the exported data.#6
cscott wants to merge 5 commits into
node-unicode:masterfrom
cscott:bidi

Conversation

@cscott

@cscott cscott commented Dec 11, 2013

Copy link
Copy Markdown
Collaborator

No description provided.

@mathiasbynens

Copy link
Copy Markdown
Collaborator

Woah, that’s a lot of changes in a single PR :)

I’ll have a look and leave some comments. Thanks!

Comment thread README.md Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any objection to making it bidi-mirroring instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection. I was just trying to match the unicode property file name, which is BidiMirroring.txt.

Presumably you'd like the type value to be 'bidi-mirroring' and the datafile name to have the hyphen, etc, as well?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. Just a nitpick of course, but I think it looks cleaner.

@mathiasbynens

Copy link
Copy Markdown
Collaborator

Awesome work! Thanks so much for this.

I left some nitpicky comments. Let me know if you’re okay with making these small changes.

Comment thread README.md

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, are these categories or properties?

You’ve added them to the properties here, which seems correct, but the code that generates the data seems to be in parse-categories.js. Am I missing something?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in parse-categories.js, some of the 'categories' are actually properties (ANY, Assigned, ASCII, etc). These are more of the same.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right!

These properties support the Unicode Bidirectional Algorithm.
@cscott

cscott commented Dec 11, 2013

Copy link
Copy Markdown
Collaborator Author

I think I've addressed all your comments. I'm going to go back and read through again to make sure.

@cscott

cscott commented Dec 11, 2013

Copy link
Copy Markdown
Collaborator Author

I'm adding some test cases while I'm at it. :) hang on.

@mathiasbynens

Copy link
Copy Markdown
Collaborator

Yay for more test cases!

We also export an array directly mapping codepoints to properties where
that makes sense: canonical category, bidi property, and the mirroring
glyph.
@cscott

cscott commented Dec 11, 2013

Copy link
Copy Markdown
Collaborator Author

Ok, I think I've got everything, and I added some test cases to boot. Let me know if I missed anything you asked me to change.

Comment thread scripts/utils.js

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why json: true here?

Edit: Ah, you probably wanted to wrap the result in quotes. jsesc has an option for that, though; jsesc(string, { wrap: true }) results in more compact output.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cscott Don’t bother, I’ve already done it in a follow-up commit. Thanks anyway!

mathiasbynens pushed a commit that referenced this pull request Dec 12, 2013
These properties support the Unicode Bidirectional Algorithm.

Ref. #6.
mathiasbynens pushed a commit that referenced this pull request Dec 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants